-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Agoric SDK integration fixes #1889
Conversation
Tagged @michaelfig for the review so at least you know about the fix for publish. We should do the same in SDK to ensure dapps get correct types from npm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGETM -- looks good enough to me.
I will note that the change to importing all types inline at the point of use leads to the type notation being much less usable than before. However, I (almost) understand why this is necessary, and that we have no other reasonable choice.
You should probably still wait for @michaelfig 's review. My approval is just FWIW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments for you to address or rebut. Otherwise, LGTM!
.github/workflows/ci.yml
Outdated
# Concurrency is 1 to avoid bizarre cross-package contamination. | ||
run: yarn run lerna exec --concurrency=1 yarn pack | ||
run: yarn lerna run pack | ||
|
||
- name: clean:types | ||
# Concurrency is 1 to avoid bizarre cross-package contamination. | ||
run: yarn lerna run clean:types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove # Concurrency is 1...
comments, or add more description to explain why it really is 1, and not 10 as I have in my Endo branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and yarn lerna run pack
does absolutely nothing if there is no explicit packageJson.scripts.pack
lifecycle script:
$ yarn lerna run pack
yarn run v1.22.19
$ /Users/michael/agoric/endo/node_modules/.bin/lerna run pack
lerna notice cli v5.6.2
lerna info versioning independent
lerna success run No packages found with the lifecycle script 'pack'
✨ Done in 0.76s.
$
Please revert to yarn lerna exec yarn pack
or simply just yarn pack
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahah! Thanks for the tip. I’m going to continue using yarn lerna run
for build:types
and clean:types
since ignoring the step on packages that provide no such script is a useful behavior, and I’m reverting to your original lines for pack
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don’t understand the concurrency concern. It’s definitely the case that build:types
needs to run from the bottom up, but it could do so concurrently. With build:types
occurring in a separate step, we could probably safely pack concurrently as well. To that end, I’m going to remove the concurrency limit and comment; please let me know if they remain necessary.
/// <reference types="ses"/> | ||
|
||
const { quote: q, bare: b, details: X, Fail } = assert; | ||
const { entries, values } = Object; | ||
const { ownKeys } = Reflect; | ||
|
||
/** @type {WeakSet<Pattern>} */ | ||
/** @type {WeakSet<import('./internal-types.js').Pattern>} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you were trying to avoid intrusive changes, but this is too much verbose noise. Can you please rename ./internal-types.js
to ./types.js
to follow the same filename convention as eventual-send
(internal=types.{js,d.ts}, external=exports.{js,d.ts})?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can line this up in this change with @erights’s blessing. It’ll be a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not foresee an objection, so I’ve merged for expedience. Do let me know if I should revert the refactor of internal-types.js to merely types.js.
aa9c140
to
ea0d31b
Compare
ea0d31b
to
b7f18a7
Compare
refs: Agoric/agoric-sdk#8514
Description
Various changes to Endo are necessary to fix integration with Agoric SDK such that Endo packages can be upgraded there.
The largest necessary change is that we must generate the types for each package based on the generated types of all dependencies. To do this, we replace prepack and postpack scripts with build:types and clean:types, then drive them from the top level before publishing.
Then, certain ambient types must be explicitly imported and the
.js
extension is required throughout.We also needed to relax some types in Exo and Patterns.
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations